Skip to content

feat: consolidate TLS flags into single --insecure option#333

Merged
mangelajo merged 20 commits intojumpstarter-dev:mainfrom
raballew:003-fix-tls-flag-naming
Apr 8, 2026
Merged

feat: consolidate TLS flags into single --insecure option#333
mangelajo merged 20 commits intojumpstarter-dev:mainfrom
raballew:003-fix-tls-flag-naming

Conversation

@raballew
Copy link
Copy Markdown
Member

@raballew raballew commented Mar 17, 2026

Summary

  • Rename --insecure-tls-config / --insecure-tls to --insecure across all CLI commands
  • Remove --insecure-login-tls and --insecure-login-http from jmp login -- --insecure now covers the entire connection chain
  • If --insecure is passed and no URL scheme is provided for login, HTTP is used; otherwise HTTPS with TLS verification disabled
  • Consistent flag naming across jmp login, jmp-admin create client/exporter, jmp-admin import client/exporter

Closes #318

Test plan

  • --insecure flag accepted and defaults to false
  • jmp login --help shows only --insecure, no --insecure-tls-* or --insecure-login-* flags
  • jmp-admin create client --help shows --insecure
  • jmp-admin create exporter --help shows --insecure
  • jmp-admin import client --help shows --insecure
  • jmp-admin import exporter --help shows --insecure
  • Insecure confirmation prompt updated
  • All 23 existing tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Updated docs and examples to use the unified --insecure flag for authentication, login, and distributed setup; examples reflect non-interactive login usage.
  • Refactor

    • Consolidated TLS/HTTP toggles into a single --insecure option across CLI flows and commands; login flow simplified to use it.
  • New Features

    • Added compact CLI output modes ("name" and "path").
  • Tests

    • Updated and added tests covering the --insecure flag and non-interactive login.
  • Chores

    • Added local tooling entries to .gitignore.

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 17, 2026

Deploy Preview for jumpstarter-docs ready!

Name Link
🔨 Latest commit 41d61c1
🔍 Latest deploy log https://app.netlify.com/projects/jumpstarter-docs/deploys/69d675eb6977760007d25ad4
😎 Deploy Preview https://deploy-preview-333--jumpstarter-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 17, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Consolidates multiple TLS/insecure CLI flags into a single --insecure flag across CLI modules, renames related parameters and confirmation helpers, updates docs, tests, and .gitignore, and extends OutputMode with NAME and PATH; includes backward-compatible aliases in common options.

Changes

Cohort / File(s) Summary
Gitignore
/.gitignore
Add local tooling ignores: .specify/, .envrc, and specs/.
Documentation
python/docs/source/getting-started/configuration/authentication.md, python/docs/source/getting-started/guides/setup-distributed-mode.md, python/packages/jumpstarter-driver-flashers/README.md
Replace --insecure-tls-config with --insecure in examples and descriptions.
CLI Common Options
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py, python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt_test.py
Introduce opt_insecure and confirm_insecure (with aliases for old names), add OutputMode.NAME/PATH, adjust output option helpers, and add tests for the --insecure flag.
CLI Login
python/packages/jumpstarter-cli/jumpstarter_cli/login.py, python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py
Unify login flags into single insecure parameter, simplify fetch_auth_config() signature and scheme handling, enforce --insecure for HTTP, and add tests covering HTTP/insecure behavior.
CLI Admin Create / Import
python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py, python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py, python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py, python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py
Rename parameters/decorators from insecure_tls_configinsecure, update internal assignments and tests to use --insecure.
Common Tests & E2E
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt_test.py, e2e/tests.bats
Add insecure-option tests and update e2e login test to use --insecure --nointeractive.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User as User
    participant CLI as CLI (jmp)
    participant Auth as AuthServer
    participant Config as LocalConfig
    rect rgba(200,200,255,0.5)
    User->>CLI: run `jmp login --insecure [--nointeractive]`
    end
    CLI->>CLI: confirm_insecure(insecure, nointeractive)
    CLI->>Auth: fetch_auth_config(login_endpoint, insecure)
    Auth-->>CLI: auth config (or error)
    CLI->>Config: build TLSConfig(insecure)
    CLI->>Auth: perform token exchange using TLSConfig
    Auth-->>CLI: token / error
    CLI->>Config: save credentials
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

enhancement, python

Suggested reviewers

  • mangelajo
  • NickCao
  • bennyz

Poem

🐰 I hopped through flags and ate three names,
One nibble later, only --insecure remains.
I tidy the trail with a jubilant thump,
CLI hops lighter — go on, take a jump! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: consolidating multiple TLS/insecure flags into a single --insecure option across the codebase.
Linked Issues check ✅ Passed The PR successfully addresses all coding requirements from issue #318: consolidates three inconsistent flags (--insecure-tls-config, --insecure-login-tls, --insecure-login-http) into a single --insecure flag with unified semantics across login, admin create, and admin import commands.
Out of Scope Changes check ✅ Passed All changes are within scope: they directly support the consolidation objective. The .gitignore update adds local tooling entries and is a minor maintenance change. Documentation updates reflect the flag rename. Tests are updated for the new flag.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@raballew raballew marked this pull request as ready for review March 17, 2026 21:07
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Around line 30-33: The code currently only adds a scheme when missing, which
allows an explicit "http://" login_endpoint to bypass the insecure flag; update
the logic in the login function (where login_endpoint and insecure are handled)
to first check for an explicit "http://" prefix and, if present and insecure is
False, fail/require confirmation (i.e., raise or surface the same
warning/confirmation path), otherwise proceed; only if no scheme is present
(login_endpoint does not start with "http://" or "https://") should you
normalize by prepending scheme = "http" if insecure else "https". Ensure you
reference and enforce the insecure flag when
login_endpoint.startswith("http://") so explicit HTTP endpoints cannot bypass
the gate.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eafff20c-ca0f-4672-9f80-b788be09119e

📥 Commits

Reviewing files that changed from the base of the PR and between ec11dc4 and a764612.

📒 Files selected for processing (12)
  • .gitignore
  • e2e/tests.bats
  • python/docs/source/getting-started/configuration/authentication.md
  • python/docs/source/getting-started/guides/setup-distributed-mode.md
  • python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
  • python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py
  • python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py
  • python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py
  • python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
  • python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt_test.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/login.py
  • python/packages/jumpstarter-driver-flashers/README.md

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py`:
- Around line 13-17: The test test_fetch_auth_config_allows_http_with_insecure
currently relies on a real network failure; instead, patch
aiohttp.ClientSession.get (or use a local async test server) to make the test
deterministic: mock ClientSession.get to capture the requested URL and ensure it
is called with "http://login.example.com" when invoking
fetch_auth_config("http://login.example.com", insecure=True), then either return
a controlled mock response or raise a specific, expected exception and assert
the call occurred; update the assertion to verify the mocked call was made and
that insecure=True path in fetch_auth_config was exercised rather than catching
any Exception.

In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Line 120: The current confirm_insecure(insecure, nointeractive) call
under-warns the case where a scheme-less login_target will fall back to plain
HTTP; update the login flow so the user sees an explicit HTTP fallback warning:
after parsing login_target into its endpoint (the code that resolves/normalizes
the target), either (A) call a new login-specific prompt (e.g.,
confirm_insecure_login(...)) that mentions "TLS disabled and will use plain HTTP
for this endpoint" or (B) extend confirm_insecure to accept a parameter like
will_use_http (or parsed_endpoint) and include the HTTP-specific text when true;
adjust the call site in login.py to pass the parsed endpoint/flag and ensure
nointeractive behavior is preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bd544a98-24bd-463d-bafd-003f1ccca04e

📥 Commits

Reviewing files that changed from the base of the PR and between a764612 and ea2a0d4.

📒 Files selected for processing (2)
  • python/packages/jumpstarter-cli/jumpstarter_cli/login.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py`:
- Around line 102-105: Update the confirmation prompt in confirm_insecure to
reflect the broader "insecure mode" semantics (not just TLS verification) by
changing the message passed to click.confirm to mention that insecure mode may
disable TLS verification and allow plain HTTP; keep the same conditional flow
and abort behavior in the confirm_insecure function so the prompt text (in
confirm_insecure) is the only change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 69258c9b-eb49-4fd8-9aa6-96bb5ef884ad

📥 Commits

Reviewing files that changed from the base of the PR and between 0d01c80 and 6de6bd4.

📒 Files selected for processing (1)
  • python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py

@mangelajo
Copy link
Copy Markdown
Member

Insecure could mean a few things: I would suggest the behavior is instead:
Rename --insecure-tls-config / --insecure-tls to --insecure-tls across all CLI commands
Remove --insecure-login-tls and --insecure-login-http from jmp login -- --insecure-tls , what is the purpose of insecure-login-http? if the user passes http it must know what he's doing, but we should probably print a warning, and ask the user unless it's a non interactive session

I think it's better to be explicit about what is insecure, tls in this case. But I would like to hear @bennyz @kirkbrauer @bkhizgiy

@raballew raballew force-pushed the 003-fix-tls-flag-naming branch 2 times, most recently from 8b91d28 to 35f6bb3 Compare March 18, 2026 11:33
@raballew raballew enabled auto-merge (squash) March 18, 2026 16:50
raballew and others added 12 commits March 20, 2026 16:01
Resolves jumpstarter-dev#318

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…flags

Consolidate all TLS/insecure flags into a single --insecure flag.
Remove --insecure-login-tls and --insecure-login-http since --insecure
covers the entire connection chain including login endpoint.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The e2e login test still used the removed --insecure-login-http flag,
causing 5 cascading test failures in CI.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace generic confirm_insecure with login-specific prompt that
  warns about both TLS disabled and plain HTTP usage
- Mock aiohttp.ClientSession in test to avoid network dependency

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n login

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@raballew raballew force-pushed the 003-fix-tls-flag-naming branch from b0cea48 to 2d9a111 Compare March 20, 2026 15:32
…eases

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kirkbrauer
Copy link
Copy Markdown
Member

kirkbrauer commented Apr 5, 2026

@mangelajo I do agree that some of these flags are unnecessarily long, especially if they are required to be used often. However, having the arguments be longer is good to discourage use, but that's a bit premature here since it's still required as part of some of our setup instructions.

If we are able to eliminate the need for many of these flags, then combining them might be fine as the user is just accepting risk whenever they use the flag such as -k in cURL. I think your proposal makes sense to unify under --insecure-tls (maybe we can add -k as well?). However, I agree that since we're not primarily a HTTP client, it is true that --insecure is ambiguous since we also have an auth mechanism.

@mangelajo
Copy link
Copy Markdown
Member

I agree with @kirkbrauer

I would propose:
--insecure-tls + -k covering them all

Then

If --insecure-tls is passed and no URL scheme is provided for login, HTTPS + insecure TLS is used; otherwise http:// schema needs to be explicitly specified.

WDYT?

Copy link
Copy Markdown
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see main thread comments

raballew and others added 2 commits April 8, 2026 10:40
Keep both the insecure flag aliases from this branch and the
validate_name function added on main.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flag now only controls TLS certificate verification and no longer
implies HTTP. When --insecure-tls is passed without a URL scheme, HTTPS
is used with verification disabled. Plain HTTP requires explicit http://
in the URL.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew and others added 2 commits April 8, 2026 12:17
…equests

The --insecure-tls flag was only applied to the initial login endpoint
fetch but not to subsequent OIDC discovery and token exchange requests,
causing TLS handshake failures in environments with self-signed certs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…unused import

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one nit with the flashers README.md change

…sue ref

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the suggestion :D

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@raballew raballew dismissed mangelajo’s stale review April 8, 2026 13:12

changes implemented

@mangelajo mangelajo disabled auto-merge April 8, 2026 13:26
@mangelajo mangelajo enabled auto-merge (squash) April 8, 2026 13:27
@mangelajo
Copy link
Copy Markdown
Member

@raballew the simplified login fails now for some reason:
https://github.com/jumpstarter-dev/jumpstarter/actions/runs/24136369402/job/70425509887?pr=333#step:6:4008

all subsequent failures seem to be related to the failed login.

Copy link
Copy Markdown
Member Author

@raballew raballew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e2e failure is because LOGIN_ENDPOINT is a plain HTTP server (login.${BASEDOMAIN}:8086), but with the flag consolidation, --insecure-tls only disables certificate verification — it still defaults to https:// when no scheme is given.

The fix is to explicitly specify the http:// scheme in the test:

-  run jmp login test-client-oidc@${LOGIN_ENDPOINT} --insecure-tls --nointeractive \
+  run jmp login test-client-oidc@http://${LOGIN_ENDPOINT} --insecure-tls --nointeractive \

The login endpoint in e2e is plain HTTP, so the test must specify
the http:// scheme explicitly since --insecure-tls only disables
certificate verification but still defaults to https://.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mangelajo mangelajo merged commit b37e463 into jumpstarter-dev:main Apr 8, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent TLS/insecure flag naming in jmp login

3 participants